Implement ReflectedRegionsBackgroundMaker #2475
Conversation
gammapy/spectrum/reflected.py
Outdated
| self.max_region_number = max_region_number | ||
|
|
||
| def _get_finder(self, observation): | ||
| finder = ReflectedRegionsFinder( |
There was a problem hiding this comment.
Another coding style suggestion. If a temp variable is just created and returned, but never used, I suggest to put return directly and avoid the temp variable. I've changed to that coding pattern already mostly throughout Gammapy
There was a problem hiding this comment.
Thanks @adonath.
A small issue with the dataset.counts.copy() that should yield a non empty CountsSpectrul
gammapy/spectrum/reflected.py
Outdated
| wcs = finder.reference_map.geom.wcs | ||
| events_off = observation.events.select_region(region_union, wcs) | ||
|
|
||
| counts_off = dataset.counts.copy() |
There was a problem hiding this comment.
This should copy the contents as well. It should be created from scratch to have an empty vector
There was a problem hiding this comment.
Ouch...that's indeed a serious bug. I guess I'd have noticed, when moving the existing tests over, but it's good to fix it now. Thanks!
There was a problem hiding this comment.
No, actually I realised CountsSpectrum.fill_events() overwrites the existing data (see here). This makes the behaviour different from Map.fill_by_coord() which adds the data. I guess we should make it consistent...
There was a problem hiding this comment.
I changed to not copy and will do the consistency change in a follow up PR (it might affect other parts in gammapy.spectrum)...
| min_distance_input="0.1 rad", | ||
| max_region_number=10000, | ||
| exclusion_mask=None, | ||
| binsz="0.01 deg", |
There was a problem hiding this comment.
If there is a default value, it is better to state it in the docstring no?
There was a problem hiding this comment.
I think at some point we decided (not sure if it's documented somewhere), that it's easier to maintain, if the default values are not repeated in the docstring again. Users always see the defaults together with the docstring when they use e.g. help(ReflectedRegionsBackgroundMaker) or ReflectedRegionsBackgroundMaker? in interactive environments. The same goes for the online documentation see e.g. here. So the information would be basically repeated...
adonath
changed the title
ReflectedRegionsBackgroundMaker
This PR implements a
ReflectedRegionsBackgroundMakerand a small first test intest_reflected.py. Other existing tests will be moved over step by step in following PRs.